Improve linter UI test runners (and fix them on Windows!) #677

BD103 · 2025-11-11T18:55:26Z

Sorry for the vague title! This PR is a refactor of our linter UI test runners (ui.rs and ui_cargo.rs) with a number of benefits:

Progress towards running UI tests on Windows by building bevy and its dependencies with DependencyBuilder (fixes Fix CI on Windows #568 (comment), which prevented Fix CI on Windows #568 from being merged)
New @exit-code: CODE annotation for Cargo UI tests, used to ensure checks pass
Support passing arguments to Cargo UI tests (e.g. cargo test --test ui_cargo -- --bless)

Here's a few notes I have on specific changes / design decisions:

Removing `test_utils`

The test_utils module had two main features:

Providing a base Config that both ui.rs and ui_cargo.rs could use, avoiding unnecessary duplication
Building and locating the compiled bevy crate so UI tests could import it

When looking over the base_config(), I realized ui.rs used all of the configuration without modification, while ui_cargo.rs essentially overwrote the entire thing. Because of that, I think it is much clearer to define each Config separately, rather than trying to use a shared base when there is little overlap. I think it worked! Compare the before-and-after for ui_cargo.rs:

bevy_cli/bevy_lint/tests/ui_cargo.rs

Lines 18 to 51 in 569ec9e

    
           let mut config = base_config("ui-cargo").unwrap(); 
        
           let defaults = config.comment_defaults.base(); 
        
           // The driver returns a '101' on error. 
        
           // This allows for any status code to be considered a success. 
        
           defaults.exit_status = None.into(); 
        
           defaults.require_annotations = None.into(); 
        
           // This sets the '--manifest-path' flag 
        
           config.program.input_file_flag = CommandBuilder::cargo().input_file_flag; 
        
           config.program.out_dir_flag = CommandBuilder::cargo().out_dir_flag; 
        
           // Do not print cargo log messages 
        
           config.program.args = vec!["--quiet".into(), "--color".into(), "never".into()]; 
        
           let current_exe_path = env::current_exe().unwrap(); 
        
           let deps_path = current_exe_path.parent().unwrap(); 
        
           let profile_path = deps_path.parent().unwrap(); 
        
           // Specify the binary to use when executing tests with this `Config` 
        
           config.program.program = profile_path.join(if cfg!(windows) { 
        
               "bevy_lint_driver.exe" 
        
           } else { 
        
               "bevy_lint_driver" 
        
           }); 
        
           config.program.program.set_file_name(if cfg!(windows) { 
        
               "bevy_lint.exe" 
        
           } else { 
        
               "bevy_lint" 
        
           }); 
        
           // this clears the default `--edition` flag 
        
           config.comment_defaults.base().custom.clear();

bevy_cli/bevy_lint/tests/ui_cargo.rs

Lines 21 to 40 in d80936b

    
           let mut config = Config { 
        
               // We need to specify the host tuple manually, because if we don't then `ui_test` will try 
        
               // running `bevy_lint -vV` to discover the host and promptly error because `bevy_lint` 
        
               // doesn't recognize the `-vV` flag. 
        
               host: Some(host_tuple()), 
        
               program: CommandBuilder { 
        
                   program: linter_path.into(), 
        
                   args: vec!["--color=never".into(), "--quiet".into()], 
        
                   out_dir_flag: Some("--target-dir".into()), 
        
                   input_file_flag: Some("--manifest-path".into()), 
        
                   envs: Vec::new(), 
        
                   cfg_flag: None, 
        
               }, 
        
               out_dir: PathBuf::from("../target/ui"), 
        
               ..Config::cargo(Path::new("tests/ui-cargo")) 
        
           }; 
        
           // We haven't found a way to get error annotations like `#~v ERROR: msg` to work, so we disable 
        
           // the requirement for them. 
        
           config.comment_defaults.base().require_annotations = None.into();

The second feature test_utils provided was building and locating libbevy.rlib. Turns out we can replace that with...

Using `DependencyBuilder`

ui_test provides a DependencyBuilder that makes it easy to build dependencies used by UI tests. DependencyBuilder provides a shift in how we manage dependencies:

`test_utils`	`DependencyBuilder`
Dependencies specified in linter's `[dev-dependencies]`	Dependencies specified by `lint-ui-dependencies` crate
Dependencies built by default when `cargo test` runs	Dependencies built only when UI tests run
Dependency `.rlib`s located by parsing `cargo build`'s JSON output (which was jank enough that I wrote a blog post about an issue I found with it, whose fix never landed because it was in #568)	Dependency `.rlib`s handled by `ui_test`
Passing dependencies to `bevy_lint_driver` with `--extern` and `-L`	Dependency flags handled by `ui_test`
Does not respect `cargo::rustc-link-search` in build scripts, causing `windows` dependency to fail to compile on Windows	Handles `cargo::rustc-link-search`: oli-obk/ui_test#220
Bevy's derive macros successfully detect that `bevy::ecs` should be imported rather than `bevy_ecs`	Bevy's derive macros cannot detect that `bevy::ecs` should be imported

Overall, using DependencyBuilder makes it so we can build and run UI tests on Windows while also making maintaining the UI tests easier. The only downside I've found was that BevyManifest isn't able to find these dependencies, so it isn't able to deduce that bevy::ecs and bevy::reflect should be imported rather than bevy_ecs and bevy_reflect. I hacked around it here, but the solution isn't amazing.

Properly query host tuple

One of the oldest issues (it's there in #32!) I ran into when introducing the UI test harness was ui_test trying to auto-detect the host tuple by calling bevy_lint_driver -vV, which the linter driver skipped because it expected bevy_lint_driver rustc -vV instead. I hacked around the issue by setting the host to an empty string Some(String::new()), but DependencyBuilder doesn't like that because it tries passing the host to cargo build and ends up running cargo build --target=. Because there's no value for --target=, Cargo exits with an error.

Because there was no avoiding it anymore, I chose to instead run rustc --print=host-tuple to get the actual host tuple (which for me returns aarch64-apple-darwin). I duplicated this solution to both ui.rs and ui_cargo.rs rather than re-introducing test_utils, but I'm not set on that decision.

`#@exit-status` in Cargo UI tests

We previously discarded the exit code of Cargo UI tests because fail tests returned exit code 101, which ui_test mistakenly thought was a compiler ICE rather than a proper error. Instead of blanketly allowing all exit codes, I wrote a small annotation that lets us specify what the exit code of a specific test should be. Now all tests intended to fail should have this at the beginning:

#@exit-status: 101

[package]
# ...

And all tests intending to pass should use the normal #@check-pass:

#@check-pass

[package]
# ...

I couldn't figure out a way to reprogram ui_test to accept 101 as the normal error code, so this is the best thing I came up with.

Argument parsing in `ui_cargo.rs`

ui_cargo.rs uses ui_test::run_tests_generic() because it needs to configure the runner to work on Cargo.toml files, not *.rs files. run_tests_generic() does not do any argument parsing, however, meaning flags like --bless were completely ignored.

Since --bless is super useful, I copied the argument parsing code from run_tests() to ui_cargo.rs. Cool stuff!

`ui_test` isn't allowed to find the host tuple itself, since it tries running `bevy_lint_driver -vV` and gets very confused when `bevy_lint_driver` silently eats the `-vV` flag because it expected `rustc`'s path instead. Because of that, we need to specify the host tuple with some default. Before, leaving it as an empty string worked fine, but in a future commit I'm going to introduce `DependencyBuilder`, which needs a valid host tuple. As such, the best solution is to properly query `rustc` for the host tuple and pass it to `ui_test`.

This makes `ui_test` build our dependencies in a separate crate, rather than expecting `bevy` to be built as a dev-dependency. This should hopefully fix #568 (comment), letting us run UI tests on Windows.

I ended up duplicating `host_tuple()`, but I personally am fine with it.

This lets us verify a specific exit code is set by `bevy_lint`, rather than just ignoring it completely.

Since `run_tests_generic()` doesn't parse arguments, I took the contents of `run_tests()` and inlined them here.

The linter's `assert!()` only works when the Rust compiler's diagnostics infra is setup.

This reverts commit 1a279ce.

BD103 · 2025-11-12T18:17:30Z

bevy_lint/Cargo.toml

-# Used to deserialize `--message-format=json` messages from Cargo.
-serde_json = "1.0.145"


We don't need serde_json anymore, as we're no longer deserializing JSON messages manually. ui_test handles that for us!

BD103 · 2025-11-12T18:17:57Z

bevy_lint/Cargo.toml

-# Used when running UI tests.
-bevy = { version = "0.17.2", default-features = false, features = [
-    "std",
-    # used for the `camera_modification_in_fixed_update` lint
-    "bevy_render",
-] }


This dependency has been moved to bevy_lint/tests/dependencies/Cargo.toml.

BD103 · 2025-11-12T18:52:30Z

bevy_lint/tests/dependencies/Cargo.toml

+name = "lint-ui-dependencies"
+description = "A dummy crate that builds dependencies that `bevy_lint`'s UI tests need to import"
+edition = "2024"
+publish = false


This line is important: once we start cargo publish-ing the CLI and linter, this will prevent us from accidentally publishing this crate :)

BD103 · 2025-11-12T18:53:42Z

bevy_lint/tests/ui.rs

+#![expect(
+    clippy::disallowed_macros,
+    reason = "`bevy_lint`'s macros are intended for lints, not tests"
+)]


This #[expect(...)] is because we are using the standard library's assert!() macro. It's fine in this situation because we can't and shouldn't use the linter's version.

BD103 · 2025-11-12T18:56:49Z

Cargo.toml

+    # A dummy crate used to build dependencies that the linter needs for its UI tests.
+    "bevy_lint/tests/dependencies",


I figured it was easiest to add ui-lint-dependencies to the workspace; this way it shares the target directory for caching and Cargo.lock for dependency sharing.

On Windows if the first `cargo test` fails, it will not be caught as long as the second `cargo test` passes. It's super jank.

BD103 added 10 commits November 11, 2025 12:50

fix: remove outdated comment

de73d13

refactor: inline base_config() into ui test runner

f906ce0

chore: build bevy using ui_test's DependencyBuilder

17b7d27

This makes `ui_test` build our dependencies in a separate crate, rather than expecting `bevy` to be built as a dev-dependency. This should hopefully fix #568 (comment), letting us run UI tests on Windows.

chore: inline base_config() into cargo ui test runner

0eed476

chore!: delete test_utils

6719961

chore: simplify cargo ui test runner

747c7b9

I ended up duplicating `host_tuple()`, but I personally am fine with it.

chore: create the #@exit-status: CODE annotation

7a81808

This lets us verify a specific exit code is set by `bevy_lint`, rather than just ignoring it completely.

chore: add comment why we disable require_annotations

331f2e3

chore: support argument parsing in cargo ui tests

d80936b

Since `run_tests_generic()` doesn't parse arguments, I took the contents of `run_tests()` and inlined them here.

BD103 added A-Linter Related to the linter and custom lints C-Testing A change to the tests D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review The PR needs to be reviewed before it can be merged labels Nov 11, 2025

BD103 added 9 commits November 12, 2025 09:58

refactor: rustfmt

e8ca61c

refactor: put host_tuple() into test_utils module

7f468f6

fix: try converting unix to windows paths on windows

1a279ce

fix: use to_owned() rather than to_string()

30386b1

fix: quiet the disallowed_macros lint for tests

7199132

The linter's `assert!()` only works when the Rust compiler's diagnostics infra is setup.

Revert "fix: try converting unix to windows paths on windows"

cf7acdf

This reverts commit 1a279ce.

refactor: rustfmt

4fb3350

chore: add comments on workspace members in root Cargo.toml

c11c493

chore: handle LF and CRLF platform differences

ec3c021

BD103 commented Nov 12, 2025

View reviewed changes

BD103 added 2 commits November 12, 2025 14:03

fix: re-add bevy to dev-dependencies for doc tests

963e824

fix: split test ci step into two separate steps

d46c6d2

On Windows if the first `cargo test` fails, it will not be caught as long as the second `cargo test` passes. It's super jank.

BD103 changed the title ~~Improve linter UI test runners~~ Improve linter UI test runners (and fix them on Windows!) Nov 12, 2025

BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review The PR needs to be reviewed before it can be merged labels Nov 16, 2025

BD103 marked this pull request as draft November 22, 2025 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve linter UI test runners (and fix them on Windows!) #677

Improve linter UI test runners (and fix them on Windows!) #677

Uh oh!

BD103 commented Nov 11, 2025

Uh oh!

BD103 Nov 12, 2025

Uh oh!

BD103 Nov 12, 2025

Uh oh!

BD103 Nov 12, 2025

Uh oh!

BD103 Nov 12, 2025

Uh oh!

BD103 Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	let mut config = base_config("ui-cargo").unwrap();

	let defaults = config.comment_defaults.base();
	// The driver returns a '101' on error.
	// This allows for any status code to be considered a success.
	defaults.exit_status = None.into();

	defaults.require_annotations = None.into();

	// This sets the '--manifest-path' flag
	config.program.input_file_flag = CommandBuilder::cargo().input_file_flag;
	config.program.out_dir_flag = CommandBuilder::cargo().out_dir_flag;
	// Do not print cargo log messages
	config.program.args = vec!["--quiet".into(), "--color".into(), "never".into()];

	let current_exe_path = env::current_exe().unwrap();
	let deps_path = current_exe_path.parent().unwrap();
	let profile_path = deps_path.parent().unwrap();

	// Specify the binary to use when executing tests with this `Config`
	config.program.program = profile_path.join(if cfg!(windows) {
	"bevy_lint_driver.exe"
	} else {
	"bevy_lint_driver"
	});

	config.program.program.set_file_name(if cfg!(windows) {
	"bevy_lint.exe"
	} else {
	"bevy_lint"
	});

	// this clears the default `--edition` flag
	config.comment_defaults.base().custom.clear();

	let mut config = Config {
	// We need to specify the host tuple manually, because if we don't then `ui_test` will try
	// running `bevy_lint -vV` to discover the host and promptly error because `bevy_lint`
	// doesn't recognize the `-vV` flag.
	host: Some(host_tuple()),
	program: CommandBuilder {
	program: linter_path.into(),
	args: vec!["--color=never".into(), "--quiet".into()],
	out_dir_flag: Some("--target-dir".into()),
	input_file_flag: Some("--manifest-path".into()),
	envs: Vec::new(),
	cfg_flag: None,
	},
	out_dir: PathBuf::from("../target/ui"),
	..Config::cargo(Path::new("tests/ui-cargo"))
	};

	// We haven't found a way to get error annotations like `#~v ERROR: msg` to work, so we disable
	// the requirement for them.
	config.comment_defaults.base().require_annotations = None.into();

		# Used to deserialize `--message-format=json` messages from Cargo.
		serde_json = "1.0.145"

		# A dummy crate used to build dependencies that the linter needs for its UI tests.
		"bevy_lint/tests/dependencies",

Improve linter UI test runners (and fix them on Windows!) #677

Are you sure you want to change the base?

Improve linter UI test runners (and fix them on Windows!) #677

Uh oh!

Conversation

BD103 commented Nov 11, 2025

Removing test_utils

Using DependencyBuilder

Properly query host tuple

#@exit-status in Cargo UI tests

Argument parsing in ui_cargo.rs

Uh oh!

BD103 Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

BD103 Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

BD103 Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

BD103 Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

BD103 Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Removing `test_utils`

Using `DependencyBuilder`

`#@exit-status` in Cargo UI tests

Argument parsing in `ui_cargo.rs`